Skip to content

module: fail closed when reading package.json is denied - #65030

Open
moeghassi wants to merge 1 commit into
nodejs:mainfrom
moeghassi:module-fail-closed-denied-package-json
Open

module: fail closed when reading package.json is denied#65030
moeghassi wants to merge 1 commit into
nodejs:mainfrom
moeghassi:module-fail-closed-denied-package-json

Conversation

@moeghassi

Copy link
Copy Markdown

Summary

GetPackageJSON() in src/node_modules.cc treated every negative return
from ReadFileSync() identically: it negative-cached the path and reported the
manifest as absent. As a result a denied read (EACCES/EPERM, or an
anti-malware/EDR block that surfaces as a failed open) is indistinguishable from
ENOENT, so resolution silently falls back to index.js and the package loads
anyway.

This means an on-endpoint scanner cannot stop a require()/import of a
quarantined package by denying/quarantining its package.json β€” Node treats the
denied manifest as "no manifest" and runs the code.

Change

  • Only ENOENT and ENOTDIR now mean "there is no package.json here"
    (unchanged behavior, still negative-cached).
  • Any other read error is treated as a security-relevant signal and throws
    ERR_ACCESS_DENIED instead of falling back β€” so a denied/quarantined manifest
    aborts resolution for every package style (default index.js, "main", and
    "exports"), not just exports-based ones. The failure is intentionally
    not negative-cached so the deny isn't latched for the process lifetime.
  • TraverseParent and GetPackageScopeConfig now stop walking parent scopes
    when an exception is pending, so the thrown error propagates instead of being
    swallowed by continuing up the tree.

Adds a POSIX regression test (chmod(0) the manifest β†’ asserts
ERR_ACCESS_DENIED and that the index.js fallback did not run; skipped on
Windows and when running as root).

Notes for reviewers

  • This is a behavior change to module resolution (fail-open β†’ fail-closed on
    non-ENOENT read errors). It may warrant a semver-major label: users with
    a legitimately unreadable ancestor directory (EACCES while walking up for
    package-type detection) would now get a thrown error instead of silent
    fallback. I'm happy to scope the throw more narrowly (e.g. only the target
    package's own manifest, not ancestor type walks) if preferred.
  • cc @nodejs/loaders @nodejs/security-wg β€” feedback on the desired posture and
    scope is very welcome.

I was unable to run a full local build to compile-check (the environment lacks
the ClangCL toolset required for the current main); CI will be the source of
truth here.

GetPackageJSON() treated every negative return from ReadFileSync() the
same way: it cached a negative result and reported the manifest as
absent. A denied read (EACCES/EPERM, or an anti-malware/EDR block that
surfaces as a failed open) was therefore indistinguishable from ENOENT,
so resolution silently fell back to index.js and the package loaded
anyway. This makes it impossible for an on-endpoint scanner to stop a
require()/import of a quarantined package by denying its manifest.

Only ENOENT and ENOTDIR now mean "no package.json here". Any other read
error is treated as a security-relevant signal and throws
ERR_ACCESS_DENIED instead of falling back, so a denied or quarantined
manifest aborts resolution for every package style (default index.js,
"main", and "exports"). The failure is intentionally not negative-cached
so the deny is not latched for the lifetime of the process.

The two parent-scope walks (TraverseParent and GetPackageScopeConfig)
now stop when an exception is pending so the thrown error propagates
instead of being swallowed by continuing up the tree.

Signed-off-by: Moe Ghasemi <moeghasemi@microsoft.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 5, 2026
MILLERMARRU

This comment was marked as low quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants